Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[R-package] remove lgb.last_error() and LGBM_GetLastError_R() #4344

Merged
merged 1 commit into from
Jun 5, 2021

Conversation

jameslamb
Copy link
Collaborator

It seems that #4163 removed the last uses of the internal helper function lgb.last_error() in the R package. This PR proposes removing that function and its C++ counterpart, LGBM_GetLastError_R(). It's no longer necessary for the R package to reach into the C++ side and get the last error, since as of #4163 and other refactoring, C++ calls will raise R errors whenever one is thrown.

#define CHECK_CALL(x) \
if ((x) != 0) { \
Rf_error(LGBM_GetLastError()); \

Removing this code should have all the usual benefits of removing unused code...faster compilation, smaller library size, reduced maintenance burden, etc.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice to see one test that caused valgrind to fail is gone.

@jameslamb jameslamb merged commit 441f38b into master Jun 5, 2021
@jameslamb jameslamb deleted the r/unused-last-error branch June 5, 2021 12:51
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants